Add SCRAM-SHA-256-PLUS support#776
Conversation
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
|
|
||
| assert error_log =~ "expected :channel_binding to be :prefer, :require or :disable" | ||
| end | ||
| end |
There was a problem hiding this comment.
How easy it is to add a test for channel_binding: :require over non-SSL?
There was a problem hiding this comment.
Easy and obvious, I'll add it right away!
There was a problem hiding this comment.
@josevalim Added that test and also added a test for :prefer. Although, the :disable and :prefer tests don't really test the actual negotiation logic. It's just a sanity test. Can be removed, or we can work on finding a way to test the actual negotiation logic.. WDYT?
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
|
|
||
| # Fallback | ||
| :error -> | ||
| {:ok, {"SCRAM-SHA-256", "y,,", ""}} |
There was a problem hiding this comment.
Should we send n,, when the mode is :prefer? Because I believe sending y,, will cause Postgres to reject the authentication
There was a problem hiding this comment.
@greg-rychlewski Thank you for catching this. n,, is the correct header here, I have pushed the change.
Thinking out loud, does it also hurt sending n,, instead of y,, when the server didn't offer -PLUS? Is there some backward compatibility benefit to this, given we used to always send n,, before? y,, is more semantically correct and lets the server know what's up, and I tested it on PG 10 that supports SCRAM but no channel binding, and it accepts y,, with no issues. Any case where this might not be backward-compatible?
There was a problem hiding this comment.
I think y,, is correct because it protects from downgrade attacks. In terms of backwards compatibility I'm not super familiar with this protocol if anything supporting SCRAM is supposed to be accepting y,,. If not this could be an issue for other databases that implement the same wire protocol like Yugabyte and Cockroach.
If this is an issue I can think of a few things:
- The default shouldn't be prefer it should be disable. But this is a pretty weak default.
- We make prefer weaker so that it doesn't protect against downgrade attacks. This seems bad
- We default prefer and let some things fail and then tell them to switch the mode if their database doesn't work with it
So all these seem kind of bad I'm hoping everything just needs to parse y,, out of the box. But we'd have to check. It would be good to see what @josevalim thinks too
There was a problem hiding this comment.
We are probably overthinking this. y,, is part of the standard SCRAM grammar, so should be a safe (& obvious) assumption that servers would do it right. For confidence, libpq has the same semantics in their SCRAM logic, and I think it can be a good standard to follow.
There was a problem hiding this comment.
I am good with that! @josevalim im approving but wont merge just to be sure you saw this convo and are ok with the conclusion as well
There was a problem hiding this comment.
@Snehil-Shah thanks for the contribution btw. This is a nice feature
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
|
💚 💙 💜 💛 ❤️ |
5 participants
This adds channel binding support for SCRAM auth handshake, enabling better security with servers that support
SCRAM-SHA-256-PLUS.Also adds a
channel_bindingknob with three modes::prefer,:require, and:disable. (all self-explanatory)I tested the flow locally on PG 16 with SSL enabled and SCRAM auth (so server advertises
SCRAM-SHA-256-PLUS) in all three of the above modes, works as expected.Proposal: https://groups.google.com/g/elixir-ecto/c/Flxp0Krz6hY